Fix union of template array-key with false losing the template - #6376
Fix union of template array-key with false losing the template#6376calebdw wants to merge 1 commit into
Conversation
|
You've opened the pull request against the latest branch 2.3.x. PHPStan 2.3 is not going to be released for months. If your code is relevant on 2.2.x and you want it to be released sooner, please rebase your pull request and change its target to 2.2.x. |
|
please see issue-bot results, as it seems to fix/affect more open issues. please add regression tests where you see fit. in addition please have a look whether CI benchmark failling is a real regression. just try to run the offending benchmarks on a local machine (CI is sometimes flaky at this) |
TypeCombinator flattened TemplateBenevolentUnionType into int|string before adding other members, so TKey|false became int|string|false instead of substituting TKey.
7dcd25b to
b37672d
Compare
|
@staabm, done 👍 |
|
@SanderMuller please review @calebdw this one looks related, wdyt? |
SanderMuller
left a comment
There was a problem hiding this comment.
@staabm the doctrine/collections failure is a true positive. The fix surfaces it. Base was silently wrong. Same for the extra Larastan failure, which nobody has flagged yet. I am not the maintainer, so the merge call is yours and Ondřej's.
The doctrine failure is correct
Line 323 is $this->elements[] = $element inside add(). Doctrine's own docblock above it says:
This breaks assumptions about the template type, but it would be a backwards-incompatible change to remove this method
I reduced it to 17 lines and dumped the type of the appended array:
BASE non-empty-array<TKey of (int|string), T>
PR non-empty-array<int|TKey of (int|string), T>
Base erased the appended int key. Writing an int key into array<TKey, T> is unsound when TKey is string. PHPStan's own tip says so: "Type int is not always the same as TKey."
There is a second one, in Larastan
Larastan is red on base too, so the count is the tell. This PR has 2 failures, #6386 has 1. The extra one:
collection-generic-static-methods.php:175
-'Illuminate\Support\Collection<string, App\User|int>'
+'Illuminate\Support\Collection<int|string, App\User|int>'
Laravel's own docblock decides it:
@template TConcatKey of array-key
@return static<TKey|TConcatKey, TValue|TConcatValue>$items is Collection<string, int>, so TKey is string. The argument is array<int, User>, so TConcatKey is int. The return is Collection<int|string, App\User|int>. That is what this PR produces. Larastan's expectation encodes the old erasure.
Both projects need their expectations updated. Neither is a regression here.
The fix is broader than the title says
I dumped every union shape on both builds:
| expression | base | this PR |
|---|---|---|
TKey|false |
int|string|false |
TKey|false |
TKey|null |
int|string|null |
TKey|null |
TKey|int |
TKey |
int|TKey |
TKey|string |
TKey |
string|TKey |
TKey|TKey |
TKey |
TKey |
int|string |
int|string |
int|string |
Rows 1 and 2 are the bug in the title. Rows 3 and 4 are a second bug, and the worse one. Base dropped a concrete member on the floor, which is a silent false negative in every variance check. Both downstream failures come from rows 3 and 4, not from the title. The body may be worth widening.
The seven linked issues, checked one by one
I ran each reporter's own playground snippet, fetched from api.phpstan.org/sample, at that sample's own level and config. Not the PR's fixtures.
| issue | base | this PR |
|---|---|---|
| phpstan/phpstan#13374 | 2 assertion failures | clean |
| phpstan/phpstan#7279 | 5 assertion failures | clean |
| phpstan/phpstan#10871 (3 samples) | Map<string, …> |
Map<int|string, …>, the reporter's stated expected output verbatim |
| phpstan/phpstan#7049 | Unable to resolve the template type TGroupKey |
gone |
| phpstan/phpstan#7423 | reported assertion fails at line 148 | gone |
| phpstan/phpstan#13192 | Collection<int, …> and Collection<string, …> |
Collection<int|string, …> both ways, still not the expectation |
| phpstan/phpstan#8268 | set() expects TKey, int|string given |
gone, a different error takes its place |
Five close cleanly. Two need a word.
phpstan/phpstan#13192 improves but its snippet still fails. The reporter expects Collection<(int|string), Apple|Orange>. The parentheses are not formatting. (int|string) is the benevolent union. I confirmed that on both builds, by dumping a declared array-key beside a declared int|string. This PR produces the plain union. Merging Collection<int, Orange> with Collection<string, Apple> substitutes both templates to concrete types, so I think plain is right and the parentheses were the reporter's guess. phpstan/phpstan#13374's reporter guessed the other way and wrote int|string, which this PR matches. Your call, but "Closes phpstan/phpstan#13192" is optimistic as it stands.
phpstan/phpstan#8268 trades one error for another. The reported false positive at line 39 is gone. Line 34 now reports $this->elements[] = $value the same way doctrine/collections does. The reporter asked for no errors at all, so that snippet is still not clean.
phpstan/phpstan#7423 keeps two unrelated errors. Line 134 is identical on both builds. Line 127 stays an error but its message grows from ArrayType<TKey of (int|string), T|V> to ArrayType<(TKey of (int|string))|VKey of (int|string), T|V>. The type is now right, the describe output is harder to read. Worth a glance if describe readability matters to you.
Tests
All 8 new fixtures fail with src/Type/TypeCombinator.php reverted to base. One failure each, no dead weight.
nsrt: 1732 tests, 8 failures TypeCombinatorTest: 1199 tests, 2 failures
Checked and could not break
instanceof TemplateBenevolentUnionTypetoinstanceof TemplateTypewidens nothing.TemplateBenevolentUnionTypeis final and is the only class that is both.TKey|TKeystill deduplicates.TKey | (int|string)still collapses toint|stringon both builds. The template is absorbed when the concrete members cover its bound.TypeCombinatorcarries no#[ShadowedByTurboExtension].turbo-ext/src/TypeCombinatorCache.cppcallsdoUnion()and calls it "the reference implementation", so there is no C++ mirror to update.- On a 4524-file corpus the error sets are identical, 3265 errors, not just the counts.
Performance
Per union, median of 11 runs of 20k unions, 3 interleaved rounds, with an untouched shape as control:
| shape | base | this PR |
|---|---|---|
TKey|false |
77.8 ms | 125.8 ms |
TKey|int |
81.2 ms | 113.8 ms |
TKey|null |
74.1 ms | 125.8 ms |
int|string (control) |
39.1 ms | 38.8 ms |
The affected shapes cost 40% to 70% more. The control is flat, so that is the change and not the harness. My first attempt at this moved the control by 54%, so I discarded it and re-ran interleaved.
End to end it is much smaller. A synthetic file of 800 such unions goes from 3.51 s to 3.83 s, so +9%, slower on 3 of 3 rounds. On a 4524-file corpus with almost no array-key templates it is flat: CPU medians 136.6 s base against 138.5 s, and the spreads overlap.
So the cost lands only on generics-heavy code. Larastan or doctrine/collections would feel it; most projects will not.
Gate and CI
Suite 21338 tests / 96449 assertions green. Self-analysis clean. phpcs clean on both touched files.
integration-tests / Integration - doctrine/collections is the only red unique to this PR. It is on neither #6383 nor #6386. The Larastan red is on both, but with one failure fewer.
Everything else belongs to the base: PHPStan (8.1, windows-latest) (red on base head), both Mutation Testing jobs, and the Rector, Larastan and phpstan-laravel integration jobs. Tests with old PHPUnit (8.1, windows-latest) is IntersectionTypeTest::testIsAcceptedBy, the known flake. I ran it on both builds locally: 10/10 green each.
For the maintainer
Three things I cannot settle.
Whether doctrine/collections and Larastan get their expectations updated before or after this merges.
Whether the Closes list should keep phpstan/phpstan#13192 and phpstan/phpstan#8268. Both improve, neither reporter's snippet ends up clean.
Whether 40% to 70% per union on the affected shapes is acceptable for the correctness it buys. I think it is, but that is your call.
|
@staabm, that looks correct (or at least the error message makes sense):
this absolutely should be closed, there is no reason why merging an |

Hello!
TypeCombinatorflattenedTemplateBenevolentUnionTypeintoint|stringbefore adding other members, soTKey|falsebecameint|string|falseinstead of substitutingTKey.Thanks!
Closes phpstan/phpstan#13374
Closes phpstan/phpstan#13192
Closes phpstan/phpstan#10871
Closes phpstan/phpstan#8268
Closes phpstan/phpstan#7423
Closes phpstan/phpstan#7279
Closes phpstan/phpstan#7049